-
-
Notifications
You must be signed in to change notification settings - Fork 663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No error raised on invalid dates #527
Conversation
Codecov Report
@@ Coverage Diff @@
## master #527 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 14 14
Lines 3385 3405 +20
Branches 235 238 +3
=====================================
+ Hits 3385 3405 +20
Continue to review full report at Codecov.
|
I would recommend being very careful with this, possibly adding a deprecation period before this starts raising an error. It's certainly not good to silently fail, but there's a good chance that people have come to rely on this behavior. |
I had opened the original issue #519 due to silent failures when passing in dates that don't exist when not ingested in ISO format (e.g. 02/30/2018). Somehow that was being parsed correctly as the year 2018, but the day/month would silently fall back to 1/1. Funny enough, this silently fails when attempting to parse 'normal' american-formatted dates!
I would be grateful for a library if, on my next deployment, something started actually properly throwing errors instead of continuing to silently fail in this weird way. I'm still confused as to why |
Why wouldn't It's quite unambiguous if someone writes:
Also, I strongly recommend starting out raising a warning rather than raising an error. I've already had people looking to opt in to the "fail silently" behavior on dateutil/dateutil#540. Start with a warning of a distinct class (so it can be easily filtered out or converted to an error as desired) and give it a bit before making a release with the error behavior on by default. |
My comment was out of a lack of ever seeing that format out in the wild, and given that the acceptable list appears to be ISO 8601 formats plus the You definitely appear to have more experience with date libraries and use cases though, so I would certainly defer to your expertise / experience on this :) |
@pganssle Instead of throwing an error on an incorrect date, you would prefer to provide a warning. In addition to giving a warning, you want the warning to be wrapped in a class so that when we decide to switch to throwing an error, it is a quick fix. A couple questions: Is there another open source example that is a good model for how to write a class to provide a warning and later throw an error? Do you have other suggestions or critiques on my implementation of this bug (other than giving a warning rather than an error)? Thank you both for the quick feedback. |
@Andrew-Katcha You can see how we do a similar thing in The reason you want to use a custom The custom warning class has nothing to do with making it easier for arrow to change it over to an error, that's just so that it's easier for downstream consumers to deal with that warning and only that warning. The reason to have a warning instead of an error at all is because right now you only see the people complaining because they want |
Regarding the implementation, I don't know Edit: I think you actually "fixed" this problem by failing before it gets a chance to try all the formats in the list. I think it's actually never necessary to set |
@pganssle |
Hi @akatcha, sorry this wasn't ever looked at properly. We're currently working on a 0.15.0 release that will solve most of these invalid date bugs. |
#519
Implemented a fix that throws a parser error when a year is incorrectly placed in a date. This fix applies in situations where a string is entered for a date.
Before change behavior:
After change behavior: